ASoC: SOF: add SOF_DBG_CHECK_SDW_PERIPHERAL debug flag#5741
ASoC: SOF: add SOF_DBG_CHECK_SDW_PERIPHERAL debug flag#5741bardliao wants to merge 1 commit intothesofproject:topic/sof-devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new SOF debug flag to optionally wait for SoundWire enumeration and filter out “ghost” peripherals listed in ACPI, avoiding machine-driver selection based on non-existent devices.
Changes:
- Introduces
SOF_DBG_CHECK_SDW_PERIPHERALdebug flag. - Adds an optional enumeration wait/verification step in
hda_sdw_machine_select()before generating ACPI link/ADR structures. - Adds a local enumeration timeout constant.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sound/soc/sof/sof-priv.h | Adds a new SOF debug flag definition controlling SoundWire peripheral presence checks. |
| sound/soc/sof/intel/hda.c | Implements the optional wait-for-enumeration logic during SoundWire default machine selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (i = 0; i < peripherals->num_peripherals; i++) { | ||
| slave = peripherals->array[i]; | ||
|
|
||
| /* Check is the peripheral is present */ |
There was a problem hiding this comment.
Typo/grammar in the new comment: "Check is the peripheral is present" should be "Check if the peripheral is present" (and keep SoundWire capitalization consistent).
| /* Check is the peripheral is present */ | |
| /* Check if the SoundWire peripheral is present */ |
| * DSPLESS_MODE is not set. | ||
| * No audio functionality when enabled. | ||
| */ | ||
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) /* Check is Soundwire peripherals are |
There was a problem hiding this comment.
Grammar/capitalization in the new flag description: "Check is Soundwire peripherals are" should be "Check if SoundWire peripherals are".
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) /* Check is Soundwire peripherals are | |
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) /* Check if SoundWire peripherals are |
| /* Check is the peripheral is present */ | ||
| if (sof_debug_check_flag(SOF_DBG_CHECK_SDW_PERIPHERAL) && !slave->dev_num_sticky) { | ||
| /* Wait for the peripheral to enumerate */ | ||
| time = wait_for_completion_timeout(&slave->enumeration_complete, | ||
| msecs_to_jiffies(ENUMERATION_TIMEOUT)); | ||
| if (!time) { | ||
| dev_warn(&slave->dev, "SoundWire peripheral is not present\n"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The new continue when a SoundWire peripheral fails to enumerate can leave a link entry with num_adr == 0 while link_mask still includes that link. Several SoundWire helper paths iterate mach_params->links until num_adr == 0 (null-terminated contract), so an empty entry can prematurely terminate parsing and ignore later valid links/devices. Consider building a present_link_mask first (after the optional wait), allocating links based on that mask (+1 for a terminating entry), and only emitting link entries for links that have at least one present peripheral (and update mach->mach_params.link_mask accordingly).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| return adr_dev; | ||
| } | ||
|
|
||
| #define ENUMERATION_TIMEOUT 3000 |
There was a problem hiding this comment.
ENUMERATION_TIMEOUT is a very generic macro name and doesn’t document the unit. Rename it to something scoped (e.g. SOF_SDW_ENUMERATION_TIMEOUT_MS) and/or add a _MS suffix so it’s clear this value is in milliseconds.
| #define ENUMERATION_TIMEOUT 3000 | |
| #define SOF_SDW_ENUMERATION_TIMEOUT_MS 3000 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return adr_dev; | ||
| } | ||
|
|
||
| #define ENUMERATION_TIMEOUT 3000 |
There was a problem hiding this comment.
ENUMERATION_TIMEOUT is a very generic macro name and the units aren’t obvious at the definition site. Since it’s passed to msecs_to_jiffies(), consider renaming to something like HDA_SDW_ENUMERATION_TIMEOUT_MS (or adding a short comment) to make the units and scope clear and reduce the risk of collisions.
| #define ENUMERATION_TIMEOUT 3000 | |
| #define ENUMERATION_TIMEOUT 3000 /* milliseconds, used with msecs_to_jiffies() for HDA SoundWire enumeration */ |
c2f293d to
21ea49d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return NULL; | ||
|
|
||
| /* | ||
| * Recalculate the link_mask as a link will be empty if all peripherals on the link are |
There was a problem hiding this comment.
There is trailing whitespace at the end of this comment line; please remove it to avoid checkpatch warnings.
| * Recalculate the link_mask as a link will be empty if all peripherals on the link are | |
| * Recalculate the link_mask as a link will be empty if all peripherals on the link are |
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) /* Check if SoundWire peripherals are | ||
| * present while selecting machine driver | ||
| */ |
There was a problem hiding this comment.
This new macro definition line is very long due to the inline block comment; consider moving the comment above the #define (or shortening it) to better match kernel style and avoid checkpatch line-length warnings.
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) /* Check if SoundWire peripherals are | |
| * present while selecting machine driver | |
| */ | |
| /* Check if SoundWire peripherals are present while selecting machine driver */ | |
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) |
There was a problem hiding this comment.
This aligned to other DBG flag defines.
| links = devm_kcalloc(sdev->dev, link_num, sizeof(*links), GFP_KERNEL); | ||
| if (!links) | ||
| return NULL; | ||
|
|
||
| /* |
There was a problem hiding this comment.
mach_params->links is expected to be terminated by an entry with num_adr == 0 (e.g. asoc_sdw_count_sdw_endpoints()/asoc_sdw_parse_sdw_endpoints() iterate for (adr_link = mach_params->links; adr_link->num_adr; adr_link++)). Here links is allocated with exactly link_num entries, so when all links have at least one enumerated peripheral there is no terminating element and the iteration can read past the end of the allocation. Allocate link_num + 1 entries (zero-initialized) and keep the last one as the terminator (or recompute link_num from the final mask and still add a terminator).
There was a problem hiding this comment.
Thanks for pointing it out. I will submit another PR to address it.
If the flag is set, the driver waits for and verifies the presence of SoundWire peripherals listed in the ACPI table. This prevents the system from probing non-existent (ghost) SoundWire devices. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
If the flag is set, the driver waits for and verifies the presence of SoundWire peripherals listed in the ACPI table. This prevents the system from probing non-existent (ghost) SoundWire devices.